Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added dbt labels as part of columns info when pushing decscriptions #41

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Added dbt labels as part of columns info when pushing decscriptions #41

wants to merge 6 commits into from

Conversation

aibunny
Copy link

@aibunny aibunny commented Feb 12, 2024 •
edited
Loading

The labels are added in this format which I noticed is commonly used:

models:
  - name: book
    description: Book mart
    columns:
      - name: author
        description: The author
        label: Author

@one-data-cookie
Copy link
Collaborator

@aibunny Thanks for contributing, appreciate it!

As mentioned in #40, I think it needs to be put under meta.label, not just label (as this wouldn't show up in dbt Docs, otherwise).

Moreover, this should be also added to the opposite way, i.e. from dbt to Superset. Could you please look into that as well?

@aibunny
Copy link
Author

aibunny commented Feb 12, 2024

@aibunny Thanks for contributing, appreciate it!

As mentioned in #40, I think it needs to be put under meta.label, not just label (as this wouldn't show up in dbt Docs, otherwise).

Moreover, this should be also added to the opposite way, i.e. from dbt to Superset. Could you please look into that as well?

Thanks lemme do that.
I did that since it fit my use-case but after testing the meta.lable it now makes more sense,
what do you mean the opposite way?

@one-data-cookie
Copy link
Collaborator

Thanks lemme do that.

Pefect, thanks!

what do you mean the opposite way?

Ignore me, I misread the code changes.

@aibunny
Copy link
Author

aibunny commented Feb 13, 2024

ok, done

one-data-cookie
one-data-cookie requested changes Feb 15, 2024
View reviewed changes
Copy link
Collaborator

@one-data-cookie one-data-cookie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aibunny Thanks again! Leaving a few comments. 😉

Also, would you mind summing up for me what sort of testing you performed?

dbt_superset_lineage/push_descriptions.py Outdated
@@ -63,7 +63,7 @@ def get_datasets_from_superset(superset, superset_db_id):
break

assert datasets, "There are no datasets in Superset!"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind removing the extra whitespaces to keep it aligned with the rest of the code?

Suggested change

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also remove the added whitespace from the rest of your changes?

dbt_superset_lineage/push_descriptions.py Outdated
column_new['description'] = description

# Check if 'meta' field exists in dbt_columns
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could merge this with a previous if to keep the code DRY? It basically does the same operations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind reflecting this one, please?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you check for expressions when getting column descriptions

@aibunny
Copy link
Author

aibunny commented Feb 16, 2024

I ran some functional tests because I didn't have any other tests to work with. Basically, I tested different cases: one with a metafield and labels, another with a metafield but no labels, and finally, a scenario with no metafield at all.

@one-data-cookie
Copy link
Collaborator

I ran some functional tests

You mean against dbt and Superset? Which versions did you use? Did it work as expected then?

@aibunny
Copy link
Author

aibunny commented Feb 17, 2024

I ran some functional tests

You mean against dbt and Superset? Which versions did you use? Did it work as expected then?

yes , I used dbt 1.7 and superset api v1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@one-data-cookie one-data-cookie one-data-cookie requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aibunny @one-data-cookie